Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

server: Use local addr var in version handler. #1256

Merged

Conversation

davecgh
Copy link
Member

@davecgh davecgh commented Aug 11, 2018

This requires #1255.

Backported from decred/dcrd#1258.

This modifies the OnVersion handler for server peers to use a local variable for the remote address of the peer in order to avoid grabbing the mutex multiple times.

There are no functional changes.

This adds a test to ensure duplicate version messages are rejected.

Backported from Decred.
This modifies the negotiation logic to ensure the callback has the
opportunity to see the message before the peer is disconnected and
improves the error handling when reading the remote version message.

It also has the side effect of ensuring the protocol version is
negotiated before sending reject messages with the exception of the
first message not being a version message since negotiation is not
possible in that case.

This is being changed because it is useful for the server to see the
message regardless in order to have the opportunity to things such as
update the address manager and reject peers that don't have desired
services.

Backported from Decred.
This modifies the OnVersion callback to allow a reject message to be
returned in which case the message will be sent to the peer and the peer
will be disconnected.

Backported from Decred.
This modifies the server connection code to reject outbound peers that
do not offer full node services.
This modifies the peer code which deals with advertising service flags
via the net address fields of the version message as follows:

- For outgoing connections:
  - Set the local netaddress services to what the local peer supports
  - Set the remote netaddress services to 0 to indicate no services as
    they are still unknown
- For incoming connections:
  - Set the local netaddress services to what the local peer supports
  - Set the remote netaddress services to the what was advertised by the
    remote peer in its version message
This exposes a new method named SetServices to the address manager which
can be used to update the services for a known address.  This will be
useful to keep known services up to date.

Backported from Decred.
This adds code to update the address manager services for a known
address to the services advertised by peers when they are connected to
via an outbound connection.  It is only done for outbound connections to
help prevent malicious behavior from inbound connections.

Backported from Decred.
This modifies the OnVersion handler for server peers to use a local
variable for the inbound status of the peer in order to avoid grabbing
the mutex multiple times.

While here, it also does some light cleanup.  There are no functional
changes.

Backported from Decred.
This changes the server peers OnVersion handler to only advertise the
server as a viable target for inbound connections when the server
believes it is close the best known tip.

Backported from Decred.
This modifies the OnVersion handler for server peers to use a local
variable for the remote address of the peer in order to avoid grabbing
the mutex multiple times.

There are no functional changes.

Backported from Decred.
@davecgh davecgh force-pushed the server_onversion_use_local_for_net_addr branch from e0b9c2b to 8c981e4 Compare September 21, 2018 03:33
@davecgh
Copy link
Member Author

davecgh commented Sep 21, 2018

Rebased.

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎖

return err
}
case <-time.After(negotiateTimeout):
p.Disconnect()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I think this might explain some issues I've seen where the connection count keeps rising (towards max connected), yet the number of stable peers queryable is much lower.

}

// Update the services if needed.
if ka.na.Services != services {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, this isn't yet useful as we don't currently store the services on disk within the addrmgr. However, it does pave the way for us properly updating the services for a peer as they change overtime once we start to store the services on disk properly.

@Roasbeef Roasbeef merged commit d2046ee into btcsuite:master Oct 13, 2018
@davecgh davecgh deleted the server_onversion_use_local_for_net_addr branch October 13, 2018 00:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants